feat: add max-depth for check command + global max-depth #791

doodlesbykumbi · 2021-12-05T22:22:52Z

max-depth for the check command limits the depth of the search, a safeguard against particularly expensive queries. This allows users more fine-grain control.

At present the approach in this PR is to

Add max-depth to the check CLI command, and the API (protobufs + HTTP query params)
When max-depth is hit for check, which is compared against the number of recursive (beyond the first) calls to checkOneIndirectionFurther, the search along the given branch halts. Check returning false means a path to a relation is has not been found up to the max-depth.
Adds a global max-depth for all read operations accessible via theserve.read.max-depth key in the config json. This global max-depth takes precedence if the request-level max-depth is less than 1 or if the global max-depth is less than the request-level max depth.

Related issue(s)

closes #703
closes #702

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

Notes for self

Try to run keto locally.
This was straightforward and so I did so via Github Codespaces. The Makefile shows me how to do most common development activities. I was able to run the cat video example with a local server.
Try to understand the structure of the code.
- The app is a CLI (based on cobra) that can serve and behave as a client.
- Serving leveraging gRPC. The version of protoc used is v3.13.0, gleamed from the auto-generated Go files.
- The "expand" command also makes use of max-depth and so I can crib from that
- Adding max-depth to "check" command requires similar changes to thosse in the "expand" command: these can be roughly categorised as docs, client, server
- Running the examples, exploring the code and tests around the "expand" command sheds light on the data model
Implement
- Make changes to docs and client, to expose maxDepth param and thread it all the way to the "check" handler
- Make sense of the "check" handler. It uses recursive depth first search?! checkOneIndirectionFurther is the method that is called recursively. NOTE: The search for keto check "*" view videos /cats/1.mp4 wastes diving 2 units of depth while the answer is present at 0 depth.
- Implement first pass and confirm it through local dev and units
- Response thoughts
  - Initial thoughts are to decrement maxDepth for each recursive call of checkOneIndirectionFurther and error when maxDepth is spent
  - Alternative approach is to NOT error but instead return the value of "allowed" up to the maxDepth

CLAassistant · 2021-12-05T22:22:56Z

All committers have signed the CLA.

zepatrik

Thanks, this looks very good already 👍
I only have a view small comments, but no hurry 😉

Regarding your open questions, I agree with you that returning an error is not the right way to go. Instead, only the branch should be aborted. We could at some point add a boolean to the response that indicates whether not all branches were fully expanded, but I am not sure that there is a good use-case for that.
Except maybe of debugging, but for that you can add a log statement instead of returning an error.

Also, I think the max-depth parameter should not be required, but instead optional. With #703 we will have a configurable fallback that will be used if no value is given.

Thanks again for looking into this.

internal/check/handler.go

proto/install.sh

internal/check/engine.go

doodlesbykumbi · 2021-12-06T19:44:01Z

@zepatrik this should be good for re-review

zepatrik

Nice, thanks for all the improvements 👍
There seem to be some not cleaned up test files, I am not sure why sqlite is sometimes keeping them...

internal/persistence/sql/TestRelationTupleSubjectTypeCheck
internal/persistence/sql/migrations/TestToSingleTableMigrator_HasLegacyTable
internal/persistence/sql/migrations/migratest/TestMigrations

internal/check/engine.go

internal/check/handler.go

internal/expand/engine.go

internal/check/engine.go

internal/x/max_depth.go

proto/ory/keto/acl/v1alpha1/check_service.proto

internal/relationtuple/definitions.go

max-depth for the check command limits the depth of the search, a safeguard against particularly expensive queries. This allows users more fine-grain control.

Make max-depth optional for check API. Add global max-depth, for which request-level only takes precedence if it is greater than 0 and less than the global-max depth.

zepatrik

Thank you very much, this looks perfect now 👍

zepatrik · 2021-12-10T10:45:49Z

cmd/check/root.go

@@ -56,6 +64,7 @@ func newCheckCmd() *cobra.Command {

 	client.RegisterRemoteURLFlags(cmd.Flags())
 	cmdx.RegisterFormatFlags(cmd.Flags())
+	cmd.Flags().Int32P(FlagMaxDepth, "d", 0, "Maximum depth of the search tree. If the value is less than 1 or greater than the global max-depth then the global max-depth will be used instead.")


I like this 👍

zepatrik · 2021-12-10T10:49:42Z

internal/check/engine_test.go

+type deps struct {
+	*relationtuple.ManagerWrapper // managerProvider
+	configProvider
+	loggerProvider
+}


Ah, now I get what you mean. I think the previous approach was fine as well, as the manager wrapper type is only a helper for testing, but this is of course the best way to do it 👍

zepatrik · 2021-12-10T10:54:55Z

Ah, the only missing thing now is the CLA 😅

doodlesbykumbi · 2021-12-10T11:12:41Z

@zepatrik CLA done

zepatrik · 2021-12-10T11:26:59Z

Thank you, CCI is somehow not running... I am looking into it.

No idea why it does not like your fork, pushed now the same branch on this original repo: https://app.circleci.com/pipelines/github/ory/keto/1693/workflows/b840a908-0587-42e5-be87-51ba4a918573

…depth

doodlesbykumbi · 2021-12-10T12:28:07Z

@zepatrik It looks like the commit chore: use npm@7 to generate lock-file v2 borked something in linting. You want me to look into it or should I leave that one to you ? Otherwise all else is green

zepatrik · 2021-12-10T12:31:23Z

Yeah, for some reason the lock file resulted in a broken state... should be fixed now though

zepatrik · 2021-12-10T13:10:32Z

Thank you very much, greatly appreciate your effort 👍

doodlesbykumbi force-pushed the check-max-depth branch 2 times, most recently from da97aaa to 600856c Compare December 5, 2021 22:25

doodlesbykumbi changed the title ~~Add max-depth for check command~~ feat: add max-depth for check command Dec 5, 2021

doodlesbykumbi force-pushed the check-max-depth branch from 600856c to ac543a5 Compare December 5, 2021 22:26

doodlesbykumbi mentioned this pull request Dec 5, 2021

Add max-depth to Check API #702

Closed

zepatrik requested changes Dec 6, 2021

View reviewed changes

internal/check/handler.go Outdated Show resolved Hide resolved

proto/install.sh Outdated Show resolved Hide resolved

internal/check/engine.go Show resolved Hide resolved

doodlesbykumbi force-pushed the check-max-depth branch from b3a76e3 to 307a9e3 Compare December 6, 2021 18:55

doodlesbykumbi changed the title ~~feat: add max-depth for check command~~ feat: add max-depth for check command + global max-depth Dec 6, 2021

doodlesbykumbi force-pushed the check-max-depth branch 2 times, most recently from ad1b533 to fc40494 Compare December 6, 2021 19:26

doodlesbykumbi requested a review from zepatrik December 7, 2021 00:16

zepatrik requested changes Dec 7, 2021

View reviewed changes

doodlesbykumbi commented Dec 9, 2021

View reviewed changes

internal/relationtuple/definitions.go Outdated Show resolved Hide resolved

doodlesbykumbi force-pushed the check-max-depth branch from 0d68502 to 6afb2b4 Compare December 9, 2021 21:52

doodlesbykumbi requested a review from zepatrik December 9, 2021 21:52

doodlesbykumbi added 5 commits December 9, 2021 21:52

feat: add max-depth for check command

a242235

max-depth for the check command limits the depth of the search, a safeguard against particularly expensive queries. This allows users more fine-grain control.

feat: Add global max-depth setting

7c40254

Make max-depth optional for check API. Add global max-depth, for which request-level only takes precedence if it is greater than 0 and less than the global-max depth.

Add installation instructions to proto README

20cf241

Set global max-depth default to 10

9744043

Add tests for global max-depth

3637e76

doodlesbykumbi force-pushed the check-max-depth branch 2 times, most recently from 632eea4 to f9e73e2 Compare December 9, 2021 21:58

zepatrik approved these changes Dec 10, 2021

View reviewed changes

zepatrik marked this pull request as ready for review December 10, 2021 10:54

Refactor and cleanup max-depth implementation

07cb3ae

doodlesbykumbi force-pushed the check-max-depth branch from 5f964d2 to 07cb3ae Compare December 10, 2021 11:04

chore: format

01c6031

zepatrik and others added 4 commits December 10, 2021 12:28

chore: use npm@7 to generate lock-file v2

54461b7

autogen(docs): generate cli docs

2102ac1

Fix internal/check tests

987e9d0

Merge remote-tracking branch 'origin/check-max-depth' into check-max-…

5097241

…depth

autogen: buf and fix package-lock.json

67280e4

autogen: build buf

7ebe427

zepatrik merged commit 1e3b63f into ory:master Dec 10, 2021

doodlesbykumbi deleted the check-max-depth branch December 10, 2021 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add max-depth for check command + global max-depth #791

feat: add max-depth for check command + global max-depth #791

doodlesbykumbi commented Dec 5, 2021 •

edited by zepatrik

Loading

CLAassistant commented Dec 5, 2021 •

edited

Loading

zepatrik left a comment

doodlesbykumbi commented Dec 6, 2021

zepatrik left a comment

zepatrik left a comment

zepatrik Dec 10, 2021

zepatrik Dec 10, 2021

zepatrik commented Dec 10, 2021

doodlesbykumbi commented Dec 10, 2021

zepatrik commented Dec 10, 2021 •

edited

Loading

doodlesbykumbi commented Dec 10, 2021 •

edited

Loading

zepatrik commented Dec 10, 2021

zepatrik commented Dec 10, 2021

feat: add max-depth for check command + global max-depth #791

feat: add max-depth for check command + global max-depth #791

Conversation

doodlesbykumbi commented Dec 5, 2021 • edited by zepatrik Loading

Related issue(s)

Checklist

Further Comments

Notes for self

CLAassistant commented Dec 5, 2021 • edited Loading

zepatrik left a comment

Choose a reason for hiding this comment

doodlesbykumbi commented Dec 6, 2021

zepatrik left a comment

Choose a reason for hiding this comment

zepatrik left a comment

Choose a reason for hiding this comment

zepatrik Dec 10, 2021

Choose a reason for hiding this comment

zepatrik Dec 10, 2021

Choose a reason for hiding this comment

zepatrik commented Dec 10, 2021

doodlesbykumbi commented Dec 10, 2021

zepatrik commented Dec 10, 2021 • edited Loading

doodlesbykumbi commented Dec 10, 2021 • edited Loading

zepatrik commented Dec 10, 2021

zepatrik commented Dec 10, 2021

doodlesbykumbi commented Dec 5, 2021 •

edited by zepatrik

Loading

CLAassistant commented Dec 5, 2021 •

edited

Loading

zepatrik commented Dec 10, 2021 •

edited

Loading

doodlesbykumbi commented Dec 10, 2021 •

edited

Loading